Skip to content

gh-115692: Add tests to increase json coverage#115693

Merged
hugovk merged 8 commits into
python:mainfrom
hugovk:tests-json-increase-coverage
Apr 14, 2024
Merged

gh-115692: Add tests to increase json coverage#115693
hugovk merged 8 commits into
python:mainfrom
hugovk:tests-json-increase-coverage

Conversation

@hugovk

@hugovk hugovk commented Feb 19, 2024

Copy link
Copy Markdown
Member

Before

Module statements missing excluded branches partial coverage
Lib/json/tool.py 41 41 5 16 0 0%
Lib/json/scanner.py 56 0 0 22 0 100%
Lib/json/init.py 65 1 1 48 5 95%
Lib/json/decoder.py 212 8 0 72 4 96%
Lib/json/encoder.py 233 5 0 134 8 96%
Total 607 55 6 292 17 90%

htmlcov-before.zip

After

Module statements missing excluded branches partial coverage
Lib/json/tool.py 41 41 5 16 0 0%
Lib/json/scanner.py 56 0 0 22 0 100%
Lib/json/init.py 65 0 1 48 3 97%
Lib/json/decoder.py 212 8 0 72 4 96%
Lib/json/encoder.py 233 4 0 134 8 97%
Total 607 53 6 292 15 91%

htmlcov-after.zip

Lib/test/test_json/test_encode_basestring.py (testing json.encoder.encode_basestring()) is based on Lib/test/test_json/test_encode_basestring_ascii.py (testing json.encoder.encode_basestring_ascii()).

I also ran https://github.com/isidentical/teyit on the modified files to use more appropriate asserts with better error messages.

@bedevere-app bedevere-app Bot added awaiting core review tests Tests in the Lib/test dir labels Feb 19, 2024
@hugovk hugovk added skip news needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Feb 24, 2024
@JelleZijlstra JelleZijlstra self-requested a review March 25, 2024 20:10
Comment thread Lib/test/test_json/test_decode.py
self.assertEqual(
result,
expect,
'{0!r} != {1!r} for {2}({3!r})'.format(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use an f-string here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was based on Lib/test/test_json/test_encode_basestring_ascii.py, so I'd left it the same.

But I've now updated the Lib/test/test_json/ files to use f-strings.

Comment thread Lib/test/test_json/test_decode.py Outdated
Comment thread Lib/test/test_json/test_decode.py Outdated
Comment thread Lib/test/test_json/test_detect_encoding.py Outdated
Comment thread Lib/test/test_json/test_encode_basestring.py Outdated
("NaN", "NAN"),
]:
self.assertEqual(
self.loads(constant, parse_constant=str.upper), expected

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply use constant.upper() instead of expected.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally prefer to have explicit expected results and avoid computing them:

https://testing.googleblog.com/2014/07/testing-on-toilet-dont-put-logic-in.html

But I can change it if you prefer.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good reason to do keep expected.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent in avoiding computations in tests we should write this more explicitly:

        self.assertEqual(self.loads("Infinity", parse_constant=str.upper),
                         "INFINITY")
        self.assertEqual(self.loads("-Infinity", parse_constant=str.upper),
                         "-INFINITY")
        self.assertEqual(self.loads("NaN", parse_constant=str.upper), "NAN")

If you want to allow some computations, you can write it as:

        for name in "Infinity", "-Infinity", "NaN":
            self.assertEqual(self.loads(name, parse_constant=str.upper),
                             name.upper())

which is also more breaf and simple that the current code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the existing version:

  • it shows the explicit inputs and outputs
  • avoids extra computations
  • we can immediately see we're asserting the function under test in the exact same way each time

@hugovk hugovk removed the needs backport to 3.11 only security fixes label Apr 2, 2024

@encukou encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, as far as I see there's one nitpick remaining.

Comment thread Lib/test/test_json/test_decode.py Outdated
("NaN", "NAN"),
]:
self.assertEqual(
self.loads(constant, parse_constant=str.upper), expected

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good reason to do keep expected.

@hugovk

hugovk commented Apr 8, 2024

Copy link
Copy Markdown
Member Author

Thanks, I'd missed Jelle's last comment.

@hugovk hugovk merged commit 8fc953f into python:main Apr 14, 2024
@hugovk hugovk deleted the tests-json-increase-coverage branch April 14, 2024 12:11
@miss-islington-app

Copy link
Copy Markdown

Thanks @hugovk for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 14, 2024
(cherry picked from commit 8fc953f)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@bedevere-app

bedevere-app Bot commented Apr 14, 2024

Copy link
Copy Markdown

GH-117867 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Apr 14, 2024
hugovk added a commit that referenced this pull request Apr 14, 2024
…117867)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants